Conversation
|
The one unfortunate thing is that I've been unable to get it to build for older iOS versions (though it builds fine for newer ones). Changing It seems to be related to this issue in the Go repo, which mentions this change where the crypto library moves to a newer API, that according to Apple's documentation is available in iOS 15.0+, which would restrict compilation to those versions. I've tried downgrading Go and the crypto version, but I still run into the same issue. |
mbevc1
left a comment
mbevc1
left a comment
There was a problem hiding this comment.
Thanks for your contribution and I'm glad to hear you got it going.
Just a side note - Darwin keychain backend has tests. The iOS file adds new backend logic with no corresponding test file. Even a stub that only compiles and skips on non-iOS would help CI catch regressions in the future.
keychain_ios.go
Outdated
| // Set the isAccessibleWhenUnlocked to the boolean value of | ||
| // KeychainAccessibleWhenUnlocked is a shorthand for setting the accessibility value. |
There was a problem hiding this comment.
Is this part of the same sentance, reads a bit weird 🤔
There was a problem hiding this comment.
This is lifted straight from the old keychain.go:
Lines 44 to 45 in 505a3f7
There was a problem hiding this comment.
I've updated this comment to read better.
| passwordFunc PromptFunc | ||
|
|
||
| isSynchronizable bool | ||
| isAccessibleWhenUnlocked bool |
There was a problem hiding this comment.
This doesn't seem to be initialised?
The keychain struct includes isSynchronizable, and Set does check it — but the init() function that constructs the struct never sets it from cfg. Looking at the Darwin implementation, cfg.KeychainSynchronizable is presumably used to populate this field. The iOS init currently omits this, meaning isSynchronizable will always be false even if a caller configures it. This looks like an oversight.
There was a problem hiding this comment.
This is already the case with keychain.go. When making this PR, I simply copied over the original keyring.go, and removed everything that was either unused or macOS-specific. This slipped by me, but it also mirrors the behavior on mac, and the behavior that was already present.
There was a problem hiding this comment.
Looking at the code, cfg.KeychainSynchronizable doesn't seem to actually be used anywhere, which I think is the root oversight. I've added support for it to both iOS & macOS.
| @@ -0,0 +1,191 @@ | |||
| //go:build ios && cgo | |||
There was a problem hiding this comment.
You comment notes that iOS 15.0+ is effectively required due to a Go runtime dependency on SecTrustCopyCertificateChain. This is a meaningful constraint for downstream users, but it's not captured anywhere in the code (no comment, no doc, no README update). It would be worth at minimum adding a comment near the build tag, e.g.:
//go:build ios && cgo
// NOTE: Due to Go's crypto library requiring SecTrustCopyCertificateChain,
// this requires iOS 15.0 or later.
There was a problem hiding this comment.
The thing is, I'm not very familiar with building in Go, especially with CGO and Apple SDKs, so I'm not 100% certain that you strictly need to use a newer iOS version, but I personally haven't been able to figure out how to get around this. I'm sure it must be possible though. Thing is, I want to be able to build for older iOS, as I made this PR with the goal of helping get another project to compile for an iOS version below 15.
I do think a comment would be a good idea, perhaps even in the README, since that's far more likely to be read. But it should be warned somewhere that macOS 12.0+ is required too, since the API is only available there.
|
I don't know if you can run iOS binaries on macOS (they seem to just get SIGKILLed), and I don't think there are any iOS runners, so I think a stub would be needed. But I'm not sure what exactly you mean by that. Just a file like this? //go:build ios
package keyring |
2 participants
I finally got iOS builds working! It turns out that only Keyring needed changes, and that go-keychain didn't need to be touched. I tested it on an iOS device and it works.
For posterity, my build command (against the basic example in the README) is:
The one thing in this PR I'm not certain about is is the change I made in
keychain_darwin.go. Strangely, that changed is needed for compilation, as despite compiling for iOS, the Darwin file would seemingly be evaluated too, and Go would be unable to find the macOS-only symbols present there and error.